-
Notifications
You must be signed in to change notification settings - Fork 8k
Fix accessing of overridden private property in get_object_vars() #20182
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
One side-effect in this PR is that this will also affect |
Right. I understand the BC concern. Although I'd also expect that get_object_vars() and foreach() behaves consistently. |
@nielsdos Right. Maybe it's ok to merge this for master then. The docs say: https://www.php.net/manual/en/language.oop5.iterations.php
I guess the child property can be thought of as "reverse shadowed". Though I don't think these semantics were ever properly described or even considered. |
Agreed. This is kinda underspecified. |
Pinging @bwoebi, as he has had opinions on previous, similar issues. ^^ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That looks right to me. For any string $a, $this->$a and get_object_properties($this)[$a] should be equivalent.
I also welcome the foreach change, as otherwise foreach ($this as $prop => $foo) { $this->$prop = $foo + 1; } or something, will affect unexpected values. If you actually need to access only child-visible properties, you always can iterate over (static fn($obj) => get_object_properties($obj))($this)
or simply (array)$this
and work with the mangled property names.
/* The property we found may be private if we have a private parent | ||
* property and a protected child property, and we're accessing the | ||
* property from the parent's scope. In that case we want to access | ||
* the parent property, not child property. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to have a shorter comment in the spirit of the two comments above.
"we are looking for a protected prop but found a private one of the same name but parent class"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I simplified the comment before merging.
Fixes GH-20177